Add int4 & uint4 types to MigraphX #3378
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3378 +/- ##
===========================================
- Coverage 92.26% 92.24% -0.02%
===========================================
Files 500 500
Lines 20057 20061 +4
===========================================
Hits 18506 18506
- Misses 1551 1555 +4 ☔ View full report in Codecov by Sentry. |
| MIGRAPHX_SHAPE_VISIT_TYPES(MIGRAPHX_SHAPE_GENERATE_ENUM_TYPES) tuple_type | ||
| MIGRAPHX_SHAPE_VISIT_TYPES(MIGRAPHX_SHAPE_GENERATE_ENUM_TYPES) tuple_type, | ||
| uint4_type, | ||
| int4_type |
There was a problem hiding this comment.
We should have a macro to list these types.
| @@ -115,6 +115,9 @@ migraphx_shape_datatype_t to_shape_type(shape::type_t t) | |||
| case shape::x: return migraphx_shape_##x; | |||
| MIGRAPHX_SHAPE_VISIT_TYPES(MIGRAPHX_DETAIL_SHAPE_CASE_CONVERT) | |||
| #undef MIGRAPHX_DETAIL_SHAPE_CASE_CONVERT | |||
| case shape::uint4_type: | |||
| case shape::int4_type: | |||
| break; | |||
There was a problem hiding this comment.
This wrong, the enum values should be exposed in the API and not throw an error.
| @@ -381,6 +383,8 @@ struct MIGRAPHX_EXPORT shape | |||
| { | |||
| switch(t) | |||
| { | |||
| case uint4_type: | |||
| case int4_type: | |||
There was a problem hiding this comment.
The tuple visitor should not be called for these types. It should throw an error.
src/shape.cpp
Outdated
| } | ||
|
|
||
| shape_impl(shape::type_t t, std::vector<std::size_t> l) | ||
| : m_type(t), m_lens(std::move(l)), m_standard(true) | ||
| { | ||
| assert(t != shape::tuple_type); | ||
| assert(t != shape::tuple_type && t != shape::uint4_type && t != shape::int4_type); |
There was a problem hiding this comment.
This assert shouldn't be changed here. It should be valid to construct a shape of int4_type otherwise whats the point of adding this type.
There was a problem hiding this comment.
Thanks. This one is definitely required.
There was a problem hiding this comment.
I meant you are correct. This constructor code, as you suggest, is definitely required for correct functioning.
(I had just quickly hacked those places to make sure the CI would pass the compile stage -- as some switch statements etc. were failing.) Thanks.
src/shape.cpp
Outdated
| @@ -744,7 +752,7 @@ std::vector<shape> flatten(const std::vector<shape>& shapes) | |||
| auto subs = flatten(s.sub_shapes()); | |||
| result.insert(result.end(), subs.begin(), subs.end()); | |||
| } | |||
| else | |||
| else if(s.type() != shape::uint4_type && s.type() != shape::int4_type) | |||
There was a problem hiding this comment.
This shouldn't be changed, we should still be able to flatten an tuples of int4_type.
|
In addition, we need to add unit tests for this type.
We also need a way to automatically insert the unpack_int4 for arguments in the onnx parser so we avoid passing it to unssuported operators(which is everything except unpack_int4). I wont be around the next couple of weeks to help design or review this, so this will have to wait until september to tackle this further. |
lakhinderwalia
changed the title
|
Also, we need update the bytes calculation to be the correct amount. We also need to throw when calling index calculations. |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
Add int4 & uint4 types to MigraphX